bench: skip non-finite metric samples when parsing exposition output - #441
Conversation
Signed-off-by: Anas Khan <anxkhn28@gmail.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces changes to filter out non-finite metrics (such as NaN and infinity) during scraping, ensuring only finite values are processed. It includes new unit and integration tests to verify this behavior. The review feedback notes that the new integration test in report_test.go uses an unbounded context, violating the repository style guide's requirement for time-bounded integration tests, and suggests using a context with a timeout instead.
| "context" | ||
| "encoding/json" | ||
| "net/http" | ||
| "net/http/httptest" | ||
| "os" | ||
| "path/filepath" | ||
| "strings" | ||
| "testing" |
There was a problem hiding this comment.
3. Test coverage must follow the pyramid
Import time to allow setting a timeout on the context used in the integration test, adhering to Rule 3.5 of the Repository Style Guide.
| "context" | |
| "encoding/json" | |
| "net/http" | |
| "net/http/httptest" | |
| "os" | |
| "path/filepath" | |
| "strings" | |
| "testing" | |
| "context" | |
| "encoding/json" | |
| "net/http" | |
| "net/http/httptest" | |
| "os" | |
| "path/filepath" | |
| "strings" | |
| "testing" | |
| "time" |
References
- Rule 3.5: Integration tests must be time bounded. Look for unbounded waits, time.Sleep longer than a few hundred milliseconds, or polling without a deadline. Prefer context.WithTimeout and t.Deadline(). (link)
| metrics, err := scrapeAll(context.Background(), []string{server.URL}) | ||
| if err != nil { | ||
| t.Fatalf("scrapeAll: %v", err) | ||
| } |
There was a problem hiding this comment.
3. Test coverage must follow the pyramid
According to Rule 3.5 of the Repository Style Guide, integration tests must be time-bounded to avoid unbounded waits. Since scrapeAll performs HTTP requests, use a context with a timeout instead of context.Background().
| metrics, err := scrapeAll(context.Background(), []string{server.URL}) | |
| if err != nil { | |
| t.Fatalf("scrapeAll: %v", err) | |
| } | |
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | |
| defer cancel() | |
| metrics, err := scrapeAll(ctx, []string{server.URL}) | |
| if err != nil { | |
| t.Fatalf("scrapeAll: %v", err) | |
| } |
References
- Rule 3.5: Integration tests must be time bounded. Look for unbounded waits, time.Sleep longer than a few hundred milliseconds, or polling without a deadline. Prefer context.WithTimeout and t.Deadline(). (link)
2 participants
What's wrong
parseExpositionininternal/bench/scrape.goreads each sample value withstrconv.ParseFloatand skips the line only when parsing fails. The comment right below the check says NaN and infinity carry no information and should be dropped, butstrconv.ParseFloathappily acceptsNaN,+Infand-Infwith no error, so those values are kept as series samples.That matters at the end of a run, not during parsing.
Snapshot.Flattencopies the samples into amap[string]float64,cmd/sam-benchembeds that map in the observation, andwritehands it tojson.MarshalIndent, which returns anUnsupportedValueErrorfor non-finite floats. The practical effect: if any scraped endpoint emits a single non-finite sample (an unavailable summary quantile, for example), the whole observation fails to serialize and nothing gets written. The workload has already run at that point, so the finite metrics and the benchmark report are lost along with the one unsupported value.The fix
internal/bench/scrape.gonow rejects the sample whenmath.IsNaN(value)ormath.IsInf(value, 0)in addition to the parse error. This matches the intent the existing comment already stated, and it drops the sample rather than substituting zero, so an unavailable measurement never turns into a misleading data point. The comment was reworded to say "infinity" since negative infinity is covered too. Standard library only, no new dependencies.Two tests come with it:
TestParseExpositionNonFiniteininternal/bench/scrape_test.gofeedsNaN,+Infand-Infalongside3,0and-2.5, checks that only the three finite samples surviveFlatten, and confirmsjson.Marshalsucceeds. Zero and an ordinary negative are included on purpose so the filter can't quietly widen.TestWriteObservationWithNonFiniteMetricsincmd/sam-bench/report_test.goserves the same body from anhttptestserver, runsscrapeAll, and callswriteagainst at.TempDirpath. Before the fixwritereturns an error and leaves no file behind; now it writes valid JSON containing both the report andfinite=3.Testing
Both packages pass.